[pull] main from databricks:main#35
Merged
Merged
Conversation
… fix CREATE, drop close-drives, keep cancel (#426) * [SEA-NodeJS] Sync execute via directResults (executeStatementDirect) The default sync path (`runAsync: false`) now calls the kernel's additive `Connection.executeStatementDirect` instead of `executeStatementCancellable`. The kernel runs ExecuteStatement with a bounded server inline wait and returns WITHOUT polling past it; the session feature-detects the arm via `awaitResult`: a fast query comes back as a terminal `Statement` (result inline) → wrapped with the operation backend's `statement` arm; a slow one as an `AsyncStatement` → the `asyncStatement` arm. Because the returned handle always corresponds to a server-owned statement: - fire-and-forget CREATE/INSERT commits (server runs it inline in the POST); - `close()` is a clean release, never a drive-to-terminal (no close-drives); - a long query stays cancellable via `op.cancel()` (~150-300ms), Thrift parity; - errors surface at `executeStatement`, matching Thrift / Python use_kernel. Requires the kernel's additive directResults execute (databricks/databricks-sql-kernel#140). `execute()` on the kernel is unchanged, so Python `use_kernel` needs no change. Regenerated napi types add `executeStatementDirect`. Validated e2e (pecotesting): CREATE fire-and-forget commits, 100k read, error at execute, mid-run cancel, close() cheap (~120ms) on an abandoned long query. Unit: SEA suite 260 passing; eslint clean. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com> * [SEA-NodeJS] Address review: test the AsyncStatement arm; type-guard; null + stale comments - F1: add coverage for the directResults Running (AsyncStatement) arm — the branch the PR exists to add. Three tests via the fake's `directReturnsRunning`: (a) a still-running query routes through the AsyncStatement arm and is driven via `status()`/`waitUntilReady()`; (b) `op.cancel()` reaches the running statement's `cancel()`; (c) contrast — a fast query routes through the terminal `Statement` arm and cancel reaches it there. Previously `directReturnsRunning` was never set, so the async arm had zero coverage. - F4: replace the `as unknown as` double-casts with a cast-free user-defined type guard `isSeaAsyncStatement` (the napi `Statement`/`AsyncStatement` are the exact alias types, so no laundering is needed). Idiomatic, narrows both arms. - F5: reject a null/undefined `direct` up front via `logAndMapError` (HiveDriverError) instead of the inconsistent `direct !== null` guard that let a null fall through to the `{statement}` arm and defer an opaque TypeError. - F3: update the stale `runAsync` selector comment (still described `executeStatementCancellable` + blocking `result()`) to directResults; fix the stale `executeStatementCancellable` comment in the test. - Document the `queryTimeout`→`wait_timeout=Ns`+CANCEL interaction (a timeout shorter than the query cancels it rather than returning the Running handle). SEA unit suite 263 passing (3 new); eslint clean. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com> * [SEA-NodeJS] Regenerate napi .d.ts JSDoc to match corrected kernel direct-path docs executeStatementDirect JSDoc now reflects: no wait_timeout field (server default inline wait + auto-close), the awaitResult feature-detect contract, and the queryTimeout->CANCEL interaction. Doc-only; no API surface change. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com> * [SEA-NodeJS] Make queryTimeout a no-op on SEA (stop abusing wait_timeout) `queryTimeout` was mapped to the kernel's `wait_timeout` (via `query_timeout_secs` → `wait_timeout={N}s` + CANCEL). That is semantically wrong: `wait_timeout` is the server's inline-HOLD window (how long the POST blocks for results, capped 5–50s), NOT a statement-execution timeout. Mapping queryTimeout there silently caps it at 50s, rejects <5s with HTTP 400, and conflates the inline wait with execution. Per the option's own JSDoc, `queryTimeout` is "effective only with Compute clusters; for SQL Warehouses use the STATEMENT_TIMEOUT configuration" — and SEA targets SQL Warehouses. So make it a clean no-op on the SEA backend: not forwarded to the kernel (sync directResults path no longer sends `query_timeout_secs`), and not applied as a client-side deadline (async path). Drop the now-dead `buildExecuteOptions` param and the unused `numberToInt64` import. Verified e2e: `queryTimeout: 5` on a ~35s query previously cancelled at ~5s; it now runs to completion (no-op). directResults CREATE/close/cancel unaffected. Tests: sync + async paths now assert queryTimeout is NOT forwarded; removed the obsolete Int64 client-side-deadline test. SEA suite 262 passing; eslint clean. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com> * [SEA-NodeJS] Remove the dead queryTimeout deadline plumbing (kernel field gone) Follow-on to making queryTimeout a no-op on SEA: now that the kernel's `query_timeout_secs` field is removed (databricks/databricks-sql-kernel#140) and SeaSessionBackend no longer passes a timeout, the SeaOperationBackend client-side deadline is dead code. Remove it: - `SeaOperationBackend`: drop the `queryTimeoutSecs` option, the `queryTimeoutMs` field, and the async poll-loop deadline enforcement (the `OperationStateError(Timeout)` + best-effort-cancel branch). - Regenerated napi `.d.ts`: `ExecuteOptions` no longer carries `queryTimeoutSecs`. - Tests: remove the obsolete client-side-deadline test; drop the `queryTimeoutSecs` args from the `makeAsyncOp` / `makeSyncOp` helpers. - Comment cleanups in SeaNativeLoader. queryTimeout remains a no-op on SEA (SQL Warehouses use STATEMENT_TIMEOUT). SEA unit suite 261 passing; eslint clean; e2e directResults (CREATE/close/cancel) unaffected. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com> * style: prettier-format SeaOperationBackend + execution.test after queryTimeout removal CI runs `prettier . --check` (not just eslint); the deadline-plumbing removal left two files needing a prettier reflow. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com> --------- Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )